Code Review in Progress

• Gather context and understand request
• Read changed files and diff
• Check localization correctness and completeness
• Verify model-boundary safety claim
• Check for reuse/simplification opportunities
• Review test coverage
• Post final review

View job run

Claude finished @Hmbown's task in 3m 22s —— View job

Code Review — feat(tui): localize /mode picker and composer Vim indicator

• Gather context and understand request
• Read changed files and diff
• Check localization correctness and completeness
• Verify model-boundary safety claim
• Check for reuse/simplification opportunities
• Review test coverage
• Post final review

Summary

The harvest is clean and well-scoped. Model-boundary safety holds. One visible cosmetic regression for CJK users in the picker layout; everything else is correct.

Model-boundary safety ✓

AppMode::label() (used by context_report.rs:370, relay.rs:59, session.rs, engine.rs, debug.rs, etc.) is untouched and continues to return English &'static str. The new display_name_localized() / picker_hint_localized() / label_localized() are purely additive helpers driven by ui_locale and are never called from model-facing paths. The claim holds.

Issues

1. CJK picker alignment regression — mode_picker.rs:154 (cosmetic but user-visible)

format!(
    "{pointer} {}. {:<7}",
    mode.number(),
    mode.display_name_localized(self.locale)
),

{:<7} pads to 7 Unicode scalar values, not 7 terminal columns. For CJK mode names where every character occupies 2 columns:

• "エージェント" — 6 scalars → padded to 7 scalars, but 13 terminal columns
• "智能体" — 3 scalars → padded to 7 scalars, but 10 terminal columns

This pushes the hint span rightward by 5–6 extra columns in East Asian locales, breaking the visual alignment for the only three users who care most about localization. unicode_width is already a dependency in the crate (imported in widgets/mod.rs).

Fix — replace the padded format with explicit width-aware padding:

let name = mode.display_name_localized(self.locale);
let display_cols = UnicodeWidthStr::width(name);
let pad = " ".repeat(7usize.saturating_sub(display_cols));
format!("{pointer} {}. {name}{pad}", mode.number()),

Fix this →

2. display_name() still present and unlocalized (out of scope, follow-up)

config.rs:1613–1615 uses mode.display_name() in the command response text:

"Switched to Agent mode."   // always English, even for ja/zh-Hans users
"Already in Plan mode."

Not a regression (was English before), but worth a follow-up i18n pass for the /mode command response. No action needed in this PR.

Localization quality

All 7 locales have translations for all 10 new MessageIds. The translations accurately convey semantics:

• YOLO hints correctly describe auto-approve + shell access across all locales
• CJK translations helpfully add "(フルアクセス)" / "(完全访问)" / "(完全存取)" to reinforce the risk — minor divergence from the English but beneficial
• The Vietnamese "-- TRỰC QUAN --" for -- VISUAL -- is semantically loose (literally "intuitive/direct", not "visual selection"), but acceptable given Vim terminology doesn't have a canonical Vietnamese translation

Tests ✓

• shipped_first_pack_has_no_missing_core_messages — passes because all 10 IDs are added to ALL_MESSAGE_IDS and all 7 locale functions have the new arms
• mode_picker_strings_are_translated_in_non_english_locales — correctly tests only sentence-type messages (prompt + hints); mode names are excluded because "YOLO" and "Plan" legitimately stay identical across some locales
• Existing mode picker tests updated with Locale::En arg — no behavior change

Dead code / cleanup ✓

No remaining callers of VimMode::label() or AppMode::picker_hint() — both are cleanly gone. The display_name() unlocalized method remains but is used appropriately for non-picker contexts.

Verdict: One fix needed for CJK display-width alignment before this is pixel-perfect. Everything else is correct and the model-boundary claim is solid.

Claude finished @Hmbown's task in 1m 34s —— View job

Re-review Complete

• Gather context and understand request
• Check if CJK alignment fix from prior review was applied
• Re-read changed files for any new issues
• Post updated review

Updated Review — feat(tui): localize /mode picker and composer Vim indicator

This sync addresses the one issue flagged in the prior review. All checks pass.

CJK alignment fix ✓ — mode_picker.rs:152–158

The {:<7} scalar-count padding has been replaced with the UnicodeWidthStr::width-based approach suggested in the last review:

let name = mode.display_name_localized(self.locale);
// Pad by terminal columns, not scalar count, so wide (CJK) mode
// names keep the hint column aligned.
let pad = " ".repeat(7usize.saturating_sub(UnicodeWidthStr::width(name)));

This correctly computes display-column width for CJK mode names (e.g. "エージェント" = 12 cols, not 6 scalars). Alignment is now pixel-perfect across all 7 locales.

Composer Vim indicator also correct ✓ — widgets/mod.rs:2143–2169

composer_top_right_chrome() calls label_localized(app.ui_locale) at both call sites and measures the result with UnicodeWidthStr::width(vim_label) before placing it. CJK -- NORMAL/INSERT -- variants will not overflow the border area.

Model-boundary safety ✓

AppMode::label() (app.rs:935) remains an English &'static str unchanged. The new display_name_localized(), picker_hint_localized(), and label_localized() helpers are purely additive, driven by ui_locale, and unreachable from model-facing paths. The invariant holds.

Localization completeness ✓

All 10 MessageIds present and correctly placed in all 7 shipped locales (en, ja, zh-Hans, zh-Hant, pt-BR, es-419, vi). The exhaustive match arms in each locale function ensure any missing arm is a compile error.

Tests ✓

• shipped_first_pack_has_no_missing_core_messages — covers all 10 new IDs
• mode_picker_strings_are_translated_in_non_english_locales — verifies no English leak for sentence-type messages (prompt + hints)
• Picker unit tests updated with Locale::En arg — no behavior regression

Verdict: Ready to merge. The CJK display-width fix is correct and complete; no remaining issues.

Heads-up for merge: please rebase or merge-commit, not squash. This commit carries Harvested from PR #2239 by @gordonlu (and gordonlu is correctly the author), but a squash rewrites the body and can drop that line — silently losing both the machine credit and the auto-close for #2239. Rebase keeps both intact (just demonstrated on #3529 → #3524 auto-closing @cyq1017 with credit). The rule is now written down in #3533.

Minor, separate: the commit's committer is Claude <noreply@anthropic.com> with a Claude-Session: trailer — bot traces the credit gate doesn't inspect (it only checks author + co-author trailers). Worth stripping before merge to match the no-bot-trailer rule in AGENTS.md; it doesn't affect gordonlu's credit.

Claude finished @Hmbown's task in 1m 35s —— View job

Bot Trailer — Already Clean

• Gather context and understand request
• Inspect commit for Claude-Session: trailer and bot committer
• Confirm current state is compliant — no action needed

The synchronize event was triggered by an amended push that already resolved the issue Hunter flagged. The commit is now fully compliant:

No Co-authored-by: or Claude-Session: trailers are present in the commit body. The check-coauthor-trailers.py gate will pass. Credit attribution and the Harvested from PR #2239 auto-close line are intact.

The branch is ready to merge via rebase or merge-commit (not squash), as Hunter noted.

Both addressed:

• Rebased clean onto current main (the 3 commits that landed since had no conflict) and will merge via rebase, not squash — keeping the Harvested from PR #2239 by @gordonlu line so machine credit + the feat: i18n Phase 1-4b wiring + rebase compile fixes #2239 auto-close survive (per docs: require rebase/merge-commit (not squash) for harvested PRs #3533).
• Stripped the bot traces: dropped the Claude-Session: trailer; committer is now gordonlu as well (author was already gordonlu). No bot identity on the commit.

I've got the merge — will land it via rebase as soon as CI is green, so please leave the merge button to me to avoid an accidental squash.

Generated by Claude Code